Support Async Profiler Feature #203

zhengziyi0117 · 2024-10-24T13:33:51Z

Add the sub-command profiling async for async-profiler query API

Here are some examples

# Create a async-profiler task
swctl profiling async create --service-name=service-name --duration=60 --events=cpu,alloc \ 
	--instance-name-list=instance-name1,instance-name2 --exec-args=interval=50ms
# Query all async-profiler tasks
swctl profiling async list --service-name=service-name
# Query task progress, including task logs and successInstances and errorInstances
swctl profiling async progress --task-id=task-id
# Query the flame graph produced by async-profiler
swctl profiling async analysis --service-name=service-name --task-id=task-id \
	--instance-name-list=instance-name1,instance-name2 --event=execution_sample

wu-sheng · 2024-10-24T13:38:17Z

@mrproliu We don't have backend ready, as we need this to verify the backend. Please review from codes as much as possible.

wu-sheng · 2024-10-24T13:38:37Z

@zhengziyi0117 Please fix CI.

assets/graphqls/profiling/asyncprofiler/GetTaskList.graphql

mrproliu · 2024-10-25T00:28:54Z

internal/commands/profiling/asyncprofiler/asyncprofiler.go

+var Command = &cli.Command{
+	Name:      "asyncprofiler",
+	Usage:     "async profiler related sub-command",
+	UsageText: `todo`,


Please update the usage text.

mrproliu · 2024-10-25T00:31:45Z

internal/commands/profiling/asyncprofiler/asyncprofiler.go

+)
+
+var Command = &cli.Command{
+	Name:      "asyncprofiler",


If this command is under the profiling command, should we remove the profiler suffix?
For now, the command should be: swctl profiling asyncprofiler, I think it could be: swctl profiling async is much clear, what do you think?

mrproliu · 2024-10-25T00:36:25Z

internal/commands/profiling/asyncprofiler/create.go

+			eventTypes = append(eventTypes, query.AsyncProfilerEventType(upperCaseEvent))
+		}
+
+		execArgs := ctx.String("exec-args")


Should we make the type of execArgs as *string, and assign the value when judgment the ctx.String is not an empty string? You will get an empty string even if the exec-args is not declared.

mrproliu · 2024-10-25T00:44:10Z

internal/commands/profiling/asyncprofiler/create.go

+		eventTypes := make([]query.AsyncProfilerEventType, 0)
+		for _, event := range events {
+			upperCaseEvent := strings.ToUpper(event)
+			eventTypes = append(eventTypes, query.AsyncProfilerEventType(upperCaseEvent))
+		}


It is best to create an enum model to obtain the event types, and the CLI should verify whether we have an enum value. You can refer to https://github.com/apache/skywalking-cli/blob/master/internal/model/type.go. However, the type you choose should be a slice, which is slightly different from the existing reference.

mrproliu · 2024-10-25T00:50:44Z

internal/commands/profiling/asyncprofiler/getAnalyze.go

+			&cli.StringFlag{
+				Name:     "event",
+				Usage:    "which event types this task needs to collect.",
+				Required: true,
+			},


The enum should be a new model. Instant of a string value without any type check.

mrproliu · 2024-10-25T00:51:13Z

internal/commands/profiling/asyncprofiler/getAnalyze.go

+
+		request := &query.AsyncProfilerAnalyzationRequest{
+			TaskID:      ctx.String("task-id"),
+			InstanceIds: ctx.StringSlice("service-instance-ids"),


Please use String and split by self.

mrproliu · 2024-10-25T00:55:17Z

internal/commands/profiling/asyncprofiler/getTaskList.go

+		startTime := ctx.Int64("start-time")
+		endTime := ctx.Int64("end-time")
+		limit := ctx.Int("limit")


Let us declare the startTime, endTime, and limit as *int64 or *int first, otherwise the value will be 0.

internal/commands/profiling/asyncprofiler/getTaskProgress.go

mrproliu · 2024-10-25T00:57:13Z

internal/commands/profiling/asyncprofiler/getTaskProgress.go

+
+var getTaskProgressCommand = &cli.Command{
+	Name:    "progress",
+	Aliases: []string{"logs", "p"},


Can it be used as swctl profiling asyncprofiler logs? I think it's a difference in the meaning of progress.

mrproliu · 2024-10-25T01:20:05Z

@zhengziyi0117 Also, please update the CHANGES.md

Co-authored-by: mrproliu <[email protected]>

lujiajing1126 · 2024-10-26T07:40:59Z

Pls run golangci-lint locally to ensure it can pass CI

mrproliu · 2024-10-27T14:15:13Z

internal/commands/interceptor/instance.go

+	instanceNameFlagName      = "instance-name"
+	destInstanceIDFlagName    = "dest-instance-id"
+	destInstanceNameFlagName  = "dest-instance-name"
+	InstanceIDSliceFlagName   = "instance-id-slice"


We usually use "list" instant of "slice"

mrproliu · 2024-10-27T14:20:33Z

internal/commands/profiling/asyncprofiler/getAnalyze.go

+Examples:
+1. Query the flame graph produced by async-profiler
+$ swctl profiling async analysis --service-name=service-name --task-id=task-id \
+	--service-instance-ids=instance-name1,instance-name2 --event=execution_sample`,


Please update all incorrect name of the parameter.

wu-sheng · 2024-10-28T02:08:44Z

Could you update the PR description to add which commands are you adding, and put some examples(input and output) in it?

zhengziyi added 3 commits October 23, 2024 19:57

feat: add asyncprofiler support

d2355a0

fix: alias

37550c6

feat: support asyncprofiler

8e10fe0

wu-sheng requested a review from mrproliu October 24, 2024 13:37

wu-sheng added this to the 0.15.0 milestone Oct 24, 2024

wu-sheng added the feature this pull request provides new feature label Oct 24, 2024

mrproliu reviewed Oct 25, 2024

View reviewed changes

mrproliu and others added 16 commits October 25, 2024 09:20

Merge branch 'master' into master

50b7ba4

Update assets/graphqls/profiling/asyncprofiler/GetTaskList.graphql

5694b3a

Co-authored-by: mrproliu <[email protected]>

Update internal/commands/profiling/asyncprofiler/getAnalyze.go

b8af7e4

Co-authored-by: mrproliu <[email protected]>

fix: license header

ecd88bd

Merge remote-tracking branch 'origin/master'

54d3486

Update internal/commands/profiling/asyncprofiler/getTaskProgress.go

071d2f1

Co-authored-by: mrproliu <[email protected]>

Merge remote-tracking branch 'origin/master'

ddb13fa

fix: pr comment

693a787

fix: license header

d4222fb

fix: license header

5a38d58

fix: license header

4e144c7

test: golint version

ac0314b

test: golint version

d9e2e71

fix: go ci lint

8395933

feat: add instanceSlice interceptor

63167d9

feat: update CHANGES.md

3eedb05

lujiajing1126 and others added 3 commits October 26, 2024 16:08

fix style and lint

8a00dee

fix: golint

0f6665c

fix: golint

5f9bdc0

fix: command test ci

ba0d366

mrproliu reviewed Oct 27, 2024

View reviewed changes

zhengziyi and others added 4 commits October 28, 2024 00:04

fix: arguments name

c1b9519

fix: arguments name

a20535e

fix dependency test cases

b4c5cf7

fix trace cases

9593efc

lujiajing1126 requested a review from mrproliu October 28, 2024 02:08

mrproliu approved these changes Oct 28, 2024

View reviewed changes

mrproliu merged commit bce7faa into apache:master Oct 28, 2024
7 checks passed

wu-sheng mentioned this pull request Nov 30, 2024

Update CHANGES #206

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Async Profiler Feature #203

Support Async Profiler Feature #203

zhengziyi0117 commented Oct 24, 2024 •

edited

Loading

wu-sheng commented Oct 24, 2024

wu-sheng commented Oct 24, 2024

mrproliu Oct 25, 2024

mrproliu Oct 25, 2024

mrproliu Oct 25, 2024

mrproliu Oct 25, 2024

mrproliu Oct 25, 2024

mrproliu Oct 25, 2024

mrproliu Oct 25, 2024

mrproliu Oct 25, 2024

mrproliu commented Oct 25, 2024

lujiajing1126 commented Oct 26, 2024 •

edited

Loading

mrproliu Oct 27, 2024

mrproliu Oct 27, 2024

wu-sheng commented Oct 28, 2024

Support Async Profiler Feature #203

Support Async Profiler Feature #203

Conversation

zhengziyi0117 commented Oct 24, 2024 • edited Loading

wu-sheng commented Oct 24, 2024

wu-sheng commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrproliu commented Oct 25, 2024

lujiajing1126 commented Oct 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-sheng commented Oct 28, 2024

zhengziyi0117 commented Oct 24, 2024 •

edited

Loading

lujiajing1126 commented Oct 26, 2024 •

edited

Loading